Skip to content

Conversation

@cmp0xff
Copy link
Contributor

@cmp0xff cmp0xff commented Oct 28, 2025

@cmp0xff cmp0xff added Index Related to the Index class or subclasses Numeric Operations Arithmetic, Comparison, and Logical operations Series Series data structure labels Oct 28, 2025
@cmp0xff cmp0xff mentioned this pull request Oct 28, 2025
2 tasks
@cmp0xff cmp0xff force-pushed the feature/floordiv branch 4 times, most recently from 3df344a to 674b994 Compare October 31, 2025 11:02
Copy link
Collaborator

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things:

  1. You're inconsistent when having timedelta divided by int where "divided" is either regular division or floor division, and timedelta is a single element or a sequence of some type. You have that as being not allowed in the stubs, but they work at runtime. Sometimes you do allow it, so you have to fix that.
  2. There are a few places where you say you can't detect via static typing, and I'm surprised at that.

Also, you are making changes here that are independent of supporting floordiv, namely:

  1. You changed the pyrefly override syntax
  2. You changed some imports from from pandas import Series to from pandas.core.series import Series

In the future, if you are making changes where the changes don't have to do with the main intent of the PR, make a separate PR for those changes. E.g., in the above case, you could do one for pyrefly and another to change the imports.

@cmp0xff cmp0xff marked this pull request as draft November 4, 2025 22:33
Copy link
Contributor Author

@cmp0xff cmp0xff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1. You're inconsistent when having `timedelta` divided by `int` where "divided" is either regular division or floor division, and `timedelta` is a single element or a sequence of some type.  You have that as being not allowed in the stubs, but they work at runtime.  Sometimes you do allow it, so you have to fix that.

When the runtime result has the correct dtype, this PR should give type hints. When the operation crashes at runtime or gives object as dtype, this PR should show error. This was discussed in e.g. #1431 (comment), where Index[Timedelta] (which has dtype object) is banned.

2. There are a few places where you say you can't detect via static typing, and I'm surprised at that.

Still working on them. Will ping when finished.

1. You changed the `pyrefly` override syntax

Will do separately

2. You changed some imports from `from pandas import Series` to `from pandas.core.series import Series`

Will do separately.

@Dr-Irv
Copy link
Collaborator

Dr-Irv commented Nov 5, 2025

When the runtime result has the correct dtype, this PR should give type hints. When the operation crashes at runtime or gives object as dtype, this PR should show error. This was discussed in e.g. #1431 (comment), where Index[Timedelta] (which has dtype object) is banned.

I think this situation is different. When we banned Index[Timedelta] when the dtype was object, my concern was that you were doing tests using that type, and we don't want to assume it could be created.

Here, this is about the results of the computation. If we know the result has object dtype, I think the stubs can return a result without a specified value of S1 (i.e., either Index or Index[Any]; Series or Series[Any])

@cmp0xff cmp0xff requested a review from Dr-Irv November 6, 2025 15:05
@cmp0xff cmp0xff marked this pull request as ready for review November 6, 2025 15:32
ScalarArrayIndexTimedelta | Series[Timedelta]
)

NumListLike: TypeAlias = ( # deprecated, do not use
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you create an issue to track removing this?

Copy link
Collaborator

@Dr-Irv Dr-Irv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pretty close. Only a few things left

Comment on lines 155 to 160

def _23() -> None: # pyright: ignore[reportUnusedFunction]
left_i.floordiv(c) # type: ignore[arg-type] # pyright: ignore[reportArgumentType,reportCallIssue]

def _24() -> None: # pyright: ignore[reportUnusedFunction]
left_i.floordiv(s) # type: ignore[arg-type] # pyright: ignore[reportArgumentType,reportCallIssue]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these 2 are returning Never, then you can do an assert_type() as you did in _25(). If we are detecting it at typing time as being invalid (which I think you are doing here), then it should be in a if TYPE_CHECKING_INVALID_USAGE block

Comment on lines 168 to 173

def _33() -> None: # pyright: ignore[reportUnusedFunction]
left_i.rfloordiv(c) # type: ignore[arg-type] # pyright: ignore[reportArgumentType,reportCallIssue]

def _34() -> None: # pyright: ignore[reportUnusedFunction]
left_i.rfloordiv(s) # type: ignore[arg-type] # pyright: ignore[reportArgumentType,reportCallIssue]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same comment as above with respect to _23() and _24()

Comment on lines 260 to 262

def _23() -> None: # pyright: ignore[reportUnusedFunction]
left_i.floordiv(c) # type: ignore[arg-type] # pyright: ignore[reportArgumentType,reportCallIssue]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be surrounded by if TYPE_CHECKING_INVALID_USAGE:

pd.Series,
)
if TYPE_CHECKING_INVALID_USAGE:
assert_type(s / left_i, Any)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

surprised that pyright isn't detecting this one

check(assert_type(left_i / c, pd.Series), pd.Series)
if TYPE_CHECKING_INVALID_USAGE:
_05 = left_i / s # type: ignore[operator] # pyright: ignore[reportOperatorIssue]
assert_type(left_i / d, Never)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wouldn't you need to put this in a function?

_0 = left_i / s # type: ignore[operator] # pyright:ignore[reportOperatorIssue]
_1 = s / left_i # type: ignore[operator] # pyright:ignore[reportOperatorIssue]
_00 = left_i / s # type: ignore[operator] # pyright:ignore[reportOperatorIssue]
_01: pd.Series = s / left_i # type: ignore[operator] # pyright:ignore[reportOperatorIssue]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why include the pd.Series annotation here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Index Related to the Index class or subclasses Numeric Operations Arithmetic, Comparison, and Logical operations Series Series data structure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants